Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[NFTs] Update attributes with offchain signature #13390

Merged
merged 31 commits into from
Feb 22, 2023

Conversation

jsidorenko
Copy link
Contributor

@jsidorenko jsidorenko commented Feb 15, 2023

This PR allows to set/update NFT's attributes by providing an offchain signature.
Attribute's deposit in that case will be taken from the origin and not the signer.

This approach significantly improves the user experience, since in a standard flow the user first needs to give a permission to modify his attributes, then the external account would need to submit an attribute update extrinsic.

A possible use-case would be the evolvable nft tickets. By doing some offchain actions, the NFT's holder could get a pre-signed update to his attributes and submit it whether he sees the value.

Cumulus Companion: paritytech/cumulus#2189

jsidorenko and others added 26 commits January 12, 2023 15:15
Co-authored-by: Squirrel <gilescope@gmail.com>
# Conflicts:
#	frame/nfts/src/weights.rs
# Conflicts:
#	frame/nfts/src/benchmarking.rs
#	frame/nfts/src/common_functions.rs
#	frame/nfts/src/lib.rs
#	frame/nfts/src/mock.rs
#	frame/nfts/src/tests.rs
#	frame/nfts/src/types.rs
#	frame/nfts/src/weights.rs
@jsidorenko jsidorenko added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 15, 2023
@ruseinov
Copy link
Contributor

This is mostly looking good to me, just asking some questions, since I'm not very familiar with the codebase of this.

@command-bot
Copy link

command-bot bot commented Feb 15, 2023

@jsidorenko Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2394914 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2394914/artifacts/download.

@jsidorenko
Copy link
Contributor Author

bot bench $ pallet dev pallet_nfts

@command-bot
Copy link

command-bot bot commented Feb 15, 2023

@jsidorenko https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2395409 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 24-e99906ad-b4f4-49d8-9ba0-8df0084689e7 to cancel this command or bot cancel to cancel all commands in this pull request.

@ruseinov
Copy link
Contributor

One other thing that I have noticed - this branch seems to be based on a branch that has already been merged to master, so the commit history is kinda messy. I know this does not matter much with squash and merge, just pointing it out as it makes things a bit confusing, though the diff is pretty clear.

@ruseinov
Copy link
Contributor

Also, if there is a tracking issue for that - it might be great to mention it in the PR description. And potentially make sure that the PR closes it.

@ruseinov
Copy link
Contributor

One other thing that I have noticed - this branch seems to be based on a branch that has already been merged to master, so the commit history is kinda messy. I know this does not matter much with squash and merge, just pointing it out as it makes things a bit confusing, though the diff is pretty clear.

This is happening, because the same branch is being used that has already been merged. For future work I would definitely make sure that the changeset is based on master to avoid confusion for both reviewers and posterity.

@ruseinov ruseinov self-requested a review February 15, 2023 11:18
Copy link
Contributor

@ruseinov ruseinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changeset looks good to me, apart from the issues I mentioned that have to do with commit history.

@jsidorenko
Copy link
Contributor Author

I agree, I needed to cherry-pick the commits. I tried to do the rebase and got into the conflicts hell, so decided just to use merge :)

@command-bot
Copy link

command-bot bot commented Feb 15, 2023

@jsidorenko Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2395409 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2395409/artifacts/download.

frame/nfts/src/types.rs Outdated Show resolved Hide resolved
jsidorenko and others added 2 commits February 22, 2023 15:11
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@jsidorenko
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b0ed48e into master Feb 22, 2023
@paritytech-processbot paritytech-processbot bot deleted the js/offchain-attributes branch February 22, 2023 13:50
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Ank4n pushed a commit that referenced this pull request Feb 28, 2023
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@mmostafas mmostafas added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Mar 6, 2023
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants